Fixed handling of long custom assertions #45
Conversation
R/assert-that.r
Outdated
| fail_default <- function(call, env) { | ||
| call_string <- deparse(call, width.cutoff = 60L) | ||
| if (length(call_string) > 1L) ch <- paste0(call_string[1L], "...") | ||
| if (length(call_string) > 1L){ |
|
I made the recommended changes. Let me know if you think anything else needs to be done! |
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
=========================================
+ Coverage 73.56% 77.27% +3.7%
=========================================
Files 9 9
Lines 87 88 +1
=========================================
+ Hits 64 68 +4
+ Misses 23 20 -3
Continue to review full report at Codecov.
|
tests/testthat/test-assert-that.R
Outdated
| test_that("assert_that handles long false assertions gracefully", { | ||
| expect_error( | ||
| assert_that(isTRUE(10 + sqrt(25) + sum(1:10) + sqrt(25) + sum(11:20) + sqrt(25) + sum(21:30) + sqrt(25) + sum(31:40) + sqrt(25) + sum(41:50))), | ||
| "^isTRUE\\(x = 10 \\+ sqrt\\(25\\) \\+ sum\\(1\\:10\\) \\+ sqrt\\(25\\) \\+ sum\\(11\\:20\\) \\+ \\.\\.\\. is not TRUE$" |
There was a problem hiding this comment.
The point of the regular expression is to make sure that you've got the right error message, not to exactly match the text. To that end, I'd suggest just matching [.]{3}.
There was a problem hiding this comment.
I disagree, actually. This test as it's written would catch a change that accidentally suppresses the first part of the error message and returns a NULL (which can happen silently in lots of settings).
A test with [.]{3} would pass if that happened and the error message was just ... is not TRUE
There was a problem hiding this comment.
Then make it a little more specific; your current test is too fragile.
There was a problem hiding this comment.
ok sure. Changed to "^isTRUE\\(.* [.]{3} is not TRUE$" to catch the idea that at least some of the code passed in the expression is returned in the result
tests/testthat/test-assert-that.R
Outdated
| ) | ||
| }) | ||
|
|
||
| test_that("assert_that handles long true assertions gracefully", { |
There was a problem hiding this comment.
I don't think you need this test. Doesn't it test the same code as the previous test?
There was a problem hiding this comment.
No. In the first test, the condition evaluates to FALSE and throws an error.
In this test, I changed the condition slightly so it evaluates to TRUE and passes. I think it's a generally useful habit to get in to test behavior on both side of a fork in the code caused by some condition being TRUE / FALSE.
There was a problem hiding this comment.
Oh in that case I think it's even less useful. Why do we expect long successful conditions to behave differently to short successful conditions?
There was a problem hiding this comment.
This would catch an issue if, for example, someone submitted a PR that tries to format the potential error message before evaluating the condition (causing deparse() to return more than one element).
I don't feel THAT strongly about it. If you tell me to drop it, I'll drop it.
There was a problem hiding this comment.
Then that would be caught by review. I don't think the point of tests is to protect against "attacks" by contributors.
There was a problem hiding this comment.
I removed the test. This is your package and your call.
But I do respectfully disagree. I'm not talking about malicious attacks by contributors. Well-meaning contributions can still make subtle breaking changes.
In my opinion, CI is a way to offload to a computer the task of checking all the things that you want to remain true about your code. Keeping all that knowledge in your head opens you up to human error (have you ever reviewed PRs before your first cup of coffee??) and makes it difficult to share review responsibility across multiple reviewers.
There was a problem hiding this comment.
In my experience, fragile tests of precise error messages have cause many more problems than they solve.
Adding tests for things that might potentially go wrong is of little value; spend time writing tests for high risk areas that are not already covered.
This PR addresses #43 . You can use the code reported there to reproduce.
Given a super long assertion like this:
see_ifreturns multiple lines of the call and results in this error:As of the changes on this PR, the behavior will be closer to what is expected:
Thank you for considering this PR.